feat: Add manual long task reporting API.#319
Conversation
Long tasks are periods in the application that stall the main thread for a specific length of time. While we can't automatically detect these in the C++ SDK, we need to provide a way for clients (including other SDKs) to manually report them. The C++ SDK takes a long task duration in Nanoseconds (following the Android SDK as an example) and sends a Long Task event to RUM. Additionally, View and Action scopes keep track of the number of Long Tasks that occur in their scope, and forward this information over to RUM when those events are sent. refs: RUM-17248
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67117400a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (command.Is<RumAddLongTaskPayload>()) { | ||
| _num_long_tasks_recorded++; |
There was a problem hiding this comment.
Count long tasks before expiring the action
When a long task is reported after a discrete action's 100 ms timeout—for example AddAction at T=0, a task starts at T=5 ms and lasts 800 ms, and AddLongTask is called at T=805 ms—the action scope closes in the earlier expiration path and returns before this new AddLongTask block runs. The long-task event is then emitted without action.id, and the action event lacks action.long_task.count, even though SendLongTaskEvent backdates the event to the task start, so frozen frames caused by user actions are not correlated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
A good callout, but likely will remain a gap in our functionality. We can't keep Actions around forever hoping to see a Long Task after the action event has expired.
This Gap likely also exists in other SDKs.
| * provided as an Attribute with ValueType::Object. | ||
| */ | ||
| DATADOG_API void AddLongTask( | ||
| uint64_t duration_ns, const Attribute& attributes = Attribute() |
There was a problem hiding this comment.
FWIW we expose a datadog::Duration (which is just an alias for std::chrono::nanoseconds) in the C++ API, which would give us stricter type safety and convenient conversion from other std::chrono formats.
That said, I'm totally fine with just using uint64_t here for the sake of simplicity.
There was a problem hiding this comment.
Completely your call here. I feel like uint64_t might be more convenient for clients, but datadog::Duration is definitely the safer option.
Just say the word and I'l change it over.
There was a problem hiding this comment.
After letting this marinate a bit more: we use int64_t internally (because it's sufficiently large, it's consistent with std::chrono defaults, and it lets us manage negative deltas without having to deal with potential underflow / signed-vs-unsigned arithmetic issues) - so we should probably use int64_t, not uint64_t, if we're going to accept raw counts at the API layer.
I'm pretty on the fence about whether we should use std::chrono-backed types from datadog/timestamp.hpp here, but I've been drawing up plans for adding custom timings, duration vitals, etc. to the RUM API, and I think there will be places where we're accepting arbitrary timestamps/durations there too.
With that in mind, I'm leaning toward using our official types more consistently. Here's what I'd propose:
- We use
datadog::Durationhere, and in any future C++ API function where we accept a value representing duration / elapsed time / etc., which should give us pretty sane modern-C++ ergonomics:- Calling
AddLongTask(850000000)is a compile error; application must be explicit about type - Calling
AddLongTask(std::chrono::nanoseconds(850000000))is accepted - Calling
AddLongTask(std::chrono::milliseconds(850))is also accepted, and is equivalent - Calling
AddLongTask(datadog::Duration{850000000})is accepted; this is fine since we explicitly warrant thatdatadog::Durationis a count of nanoseconds
- Calling
- In the C API, we use
int64_tand ensure that parameter names end in_ns- We could optionally define
dd_duration_t(as just anotherint64_ttypedef likedd_timestamp_t, with convenience functionsdd_duration_ns(int64_t),dd_duration_ms(int64_t), etc.) - it's functionally the same ABI-wise, so we're free to add that convenience typedef later if we don't care to do it now
- We could optionally define
refs: RUM-17275
Long tasks are periods in the application that stall the main thread for a specific length of time. While we can't automatically detect these in the C++ SDK, we need to provide a way for clients (including other SDKs) to manually report them.
The C++ SDK takes a long task duration in Nanoseconds (following the Android SDK as an example) and sends a Long Task event to RUM. Additionally, View and Action scopes keep track of the number of Long Tasks that occur in their scope, and forward this information over to RUM when those events are sent.